Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

9.) graph based decoding #1613

Open
wants to merge 1 commit into
base: saxelrod-delete-old-detect-code
Choose a base branch
from

Conversation

shanaxel42
Copy link
Collaborator

@shanaxel42 shanaxel42 commented Oct 15, 2019

This PR takes the work done by @gapartel in #1482 and modifies it to exist within the new spot finding modules.

  • The original PR had spot finding as a step in the graph_decoding.py file with a parameter option describing what kind of spot finder to use. I refactored the process so that we could instead take advantage of the existing spot finding module.
  • One spot finding method used h_max was missing from out existing collection so I added it as well as tests for it.
  • Since graph_decoding.py didn't actually decode the intensity values I changed some of the terminology ex. to build_traces_graph_based() instead of decode_spots()
  • The new workflow for using this method is to first find spots using the spot finding method of your choice then decode them using PerRoundMax with the parameter TraceBuildingStrategies.GRAPH. Some examples can be shown in the notebook graph_decoding.py

@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch from 2c604cf to d4d995c Compare October 15, 2019 16:50
@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch from d4d995c to 4f75af8 Compare October 15, 2019 17:27
@shanaxel42 shanaxel42 force-pushed the saxelrod-delete-old-detect-code branch from 8f72169 to 1e62cbe Compare October 15, 2019 17:37
@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch 2 times, most recently from 016f5fb to d654f41 Compare October 15, 2019 17:38
@codecov-io
Copy link

codecov-io commented Oct 15, 2019

Codecov Report

Merging #1613 into saxelrod-delete-old-detect-code will increase coverage by 0.37%.
The diff coverage is 97.25%.

Impacted file tree graph

@@                         Coverage Diff                         @@
##           saxelrod-delete-old-detect-code    #1613      +/-   ##
===================================================================
+ Coverage                            89.87%   90.24%   +0.37%     
===================================================================
  Files                                  219      221       +2     
  Lines                                 8117     8550     +433     
===================================================================
+ Hits                                  7295     7716     +421     
- Misses                                 822      834      +12
Impacted Files Coverage Δ
starfish/core/spots/DecodeSpots/trace_builders.py 100% <100%> (ø) ⬆️
starfish/core/spots/FindSpots/__init__.py 100% <100%> (ø) ⬆️
...h/core/spots/FindSpots/test/test_spot_detection.py 100% <100%> (ø) ⬆️
...spots/DecodeSpots/test/test_graph_trace_builder.py 100% <100%> (ø)
...spots/DecodeSpots/per_round_max_channel_decoder.py 100% <100%> (ø) ⬆️
starfish/core/types/_constants.py 98.27% <100%> (+0.03%) ⬆️
starfish/core/spots/FindSpots/h_max.py 84.31% <84.31%> (ø)
starfish/core/spots/DecodeSpots/util.py 98.28% <97.83%> (-1.72%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c0b6ba...2ed0053. Read the comment docs.

@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch 2 times, most recently from d702eae to 60ac93e Compare October 15, 2019 20:07
@shanaxel42 shanaxel42 added this to the 0.2.0 milestone Oct 15, 2019
@shanaxel42 shanaxel42 mentioned this pull request Oct 15, 2019
10 tasks
@shanaxel42 shanaxel42 requested a review from ttung October 15, 2019 20:20
@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch 2 times, most recently from f2cc8cf to 2879c3c Compare October 17, 2019 20:24
@shanaxel42 shanaxel42 force-pushed the saxelrod-delete-old-detect-code branch from 52e5912 to 7e24890 Compare October 18, 2019 20:51
@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch 2 times, most recently from 431ad13 to 9ac3b89 Compare October 18, 2019 21:38
@ttung ttung requested a review from dganguli October 25, 2019 18:25
Copy link
Collaborator

@ttung ttung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we really need @dganguli to review this.

@@ -122,6 +126,449 @@ def _build_intensity_table(
return intensity_table


def _build_spot_traces_per_round(
round_dataframes: Dict[int, pd.DataFrame],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
round_dataframes: Dict[int, pd.DataFrame],
round_dataframes: Mapping[int, pd.DataFrame],

if you're not going to modify, might as well make it immutable.

def _build_spot_traces_per_round(
round_dataframes: Dict[int, pd.DataFrame],
channels: Sequence[int],
rounds: Sequence[int]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does round_dataframes.keys() contain the same data?

for i in range(len(conn_comps)):
df_tmp = df.loc[conn_comps[i], :]
kdT_tmp = KDTree(df_tmp.loc[:, [Axes.ZPLANE.value, Axes.Y.value, Axes.X.value]].values)
# Check if all spots whitin a conn component are at most 1 pixels away
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Check if all spots whitin a conn component are at most 1 pixels away
# Check if all spots within a connected component are at most 1 pixels away

intensity_tables : Dict[int, IntensityTable]
Output from _merge_spots_by_round, contains mapping of intensity tables
from each round to all the spots detected in them.
channels, rounds : Sequence[int]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

channels not a param

Comment on lines +523 to +527
search_radius : int
Euclidean distance in pixels over which to search for spots in subsequent rounds.
search_radius_max : int
The maximum (euclidian) distance in pixels allowed between nodes belonging
to the same sequence
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of surprised these aren't floats

@shanaxel42 shanaxel42 force-pushed the saxelrod-delete-old-detect-code branch from 7e24890 to d0e97c4 Compare November 4, 2019 18:44
@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch from 9ac3b89 to 0ace70b Compare November 4, 2019 18:44
@shanaxel42 shanaxel42 force-pushed the saxelrod-delete-old-detect-code branch from d0e97c4 to 8c0b6ba Compare November 12, 2019 17:35
@shanaxel42 shanaxel42 force-pushed the saxelrod-graph-decoding branch from 0ace70b to 2ed0053 Compare November 12, 2019 17:35
@neuromusic neuromusic requested review from mattcai and removed request for dganguli April 27, 2020 22:54
@neuromusic
Copy link
Collaborator

hey @mattcai can you review this PR? it would be nice to get it in to 0.3.0 if its ready

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.25400% with 12 lines in your changes missing coverage. Please review.

Project coverage is 90.24%. Comparing base (8c0b6ba) to head (2ed0053).

Files Patch % Lines
starfish/core/spots/FindSpots/h_max.py 84.31% 8 Missing ⚠️
starfish/core/spots/DecodeSpots/util.py 97.83% 4 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                         Coverage Diff                         @@
##           saxelrod-delete-old-detect-code    #1613      +/-   ##
===================================================================
+ Coverage                            89.87%   90.24%   +0.37%     
===================================================================
  Files                                  219      221       +2     
  Lines                                 8117     8550     +433     
===================================================================
+ Hits                                  7295     7716     +421     
- Misses                                 822      834      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants